Skip to content

Conversation

@Thirunarayanan
Copy link
Member

  • The Jira issue number for this PR is: MDEV-38041

Description

Problem:

When an inplace ALTER operation is rolled back, InnoDB drops intermediate tables and their associated FTS internal tables. However, MariaBackup's DDL tracking can incorrectly report this as a backup failure.

The issue occurs because backup_set_alter_copy_lock() downgrades the MDL_BACKUP_DDL lock before the inplace phase of ALTER, allowing FTS internal tables to be dropped during the later phases of backup when DDL tracking is still active.

Solution:

backup_file_op_fail(): Ignore delete operations on FTS internal tables when not using --no-lock option, preventing false positive backup failures.

Release Notes

How can this PR be tested?

Run lot of rollback alter operation of fts table when mariabackup is running.
I have done some debug sync test case.

diff --git a/extra/mariabackup/backup_copy.cc b/extra/mariabackup/backup_copy.cc
index d4a90760f65..1b6a8ae48ec 100644
--- a/extra/mariabackup/backup_copy.cc
+++ b/extra/mariabackup/backup_copy.cc
@@ -1462,6 +1462,7 @@ bool backup_start(ds_ctxt *ds_data, ds_ctxt *ds_meta,
 
        corrupted_pages.backup_fix_ddl(ds_data, ds_meta);
 
+       DBUG_EXECUTE_IF("no_ddl_redo", my_sleep(100000 * 1000UL););
        // There is no need to stop slave thread before coping non-Innodb data when
        // --no-lock option is used because --no-lock option requires that no DDL or
        // DML to non-transaction tables can occur.
diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc
index 43ea8c1466d..d70e28b8e15 100644
--- a/storage/innobase/handler/handler0alter.cc
+++ b/storage/innobase/handler/handler0alter.cc
@@ -9301,6 +9301,7 @@ inline bool rollback_inplace_alter_table(Alter_inplace_info *ha_alter_info,
     }
 
     DEBUG_SYNC(ctx->trx->mysql_thd, "before_commit_rollback_inplace");
+    DBUG_EXECUTE_IF("rollback_before_redo", my_sleep(20000000););
     commit_unlock_and_unlink(ctx->trx);
     if (fts_exist)
       purge_sys.resume_FTS();

Test case:

--source include/have_innodb.inc
CREATE TABLE t1 ( col1 INT, col_text TEXT, fulltext(col_text))ENGINE=InnoDB;
INSERT INTO t1 VALUES(1, "MARIADB"), (1, "mysql");
 
set DEBUG_DBUG="+d,rollback_before_redo";
send ALTER TABLE t1 ADD PRIMARY KEY(col1);
 
let $targetdir=$MYSQLTEST_VARDIR/tmp/backup;
let $backup_log=$MYSQLTEST_VARDIR/tmp/backup.log;
--exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --target-dir=$targetdir --log-copy-interval=1 --dbug=+d,no_ddl_redo >     $backup_log 2>&1 &
 
--error ER_DUP_ENTRY
--reap
DROP TABLE t1;

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@dr-m dr-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks reasonable to me, but I think that it would be good to have regression test coverage of this. I see that you posted a test to MDEV-38041. Could that test be adjusted in some way, for example, by making use of DBUG_MARIABACKUP_EVENT and DBUG_EXECUTE_FOR_KEY, similar to what was done in 6b2da93?

Problem:
========
When an inplace ALTER operation is rolled back, InnoDB drops
intermediate tables and their associated FTS internal tables.
However, MariaBackup's DDL tracking can incorrectly report
this as a backup failure.

The issue occurs because backup_set_alter_copy_lock() downgrades the
MDL_BACKUP_DDL lock before the inplace phase of ALTER,
allowing FTS internal tables to be dropped during the
later phases of backup when DDL tracking is still active.

Solution:
========
backup_file_op_fail(): Ignore delete operations on FTS internal
tables when not using --no-lock option, preventing false
positive backup failures.
Copy link
Contributor

@dr-m dr-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I checked that the test fails if the fix is reverted. I only have some comments about formatting or comments.

Comment on lines 1072 to +1077
filename_to_spacename(name, len).c_str());
msg("DDL tracking : delete %" PRIu32 " \"%.*s\"",
space_id, int(len), name);
error= "delete";
if (fail && !opt_no_lock &&
check_if_fts_table(
filename_to_spacename(name, len).c_str())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the switch block, can we assign

const std::string spacename{filename_to_spacename(name, len)};

and replace each occurrence of filename_to_spacename(name, len) with spacename, like it was done in 63a7e4c and b1829ff?

filename_to_spacename(name, len).c_str());
msg("DDL tracking : delete %" PRIu32 " \"%.*s\"",
space_id, int(len), name);
error= "delete";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line had been indented with TAB consistently with the rest of the function. Here the indentation is being changed without any good reason.

Comment on lines +1078 to +1079
/* Ignore the FTS internal table because InnoDB does
drop intermediate table and their associative FTS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would replace "does drop" with "may drop".

Comment on lines +1083 to +1084
This leads to the FTS internal table being
dropped in the late phase of backup. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence feels redundant to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants